-
-
Notifications
You must be signed in to change notification settings - Fork 996
feat: add accessor protocol support and refactor stats/base/nanmin
#6211
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
- task: lint_filenames
status: passed
- task: lint_editorconfig
status: passed
- task: lint_markdown
status: passed
- task: lint_package_json
status: na
- task: lint_repl_help
status: passed
- task: lint_javascript_src
status: passed
- task: lint_javascript_cli
status: na
- task: lint_javascript_examples
status: passed
- task: lint_javascript_tests
status: passed
- task: lint_javascript_benchmarks
status: passed
- task: lint_python
status: na
- task: lint_r
status: na
- task: lint_c_src
status: na
- task: lint_c_examples
status: na
- task: lint_c_benchmarks
status: na
- task: lint_c_tests_fixtures
status: na
- task: lint_shell
status: na
- task: lint_typescript_declarations
status: passed
- task: lint_typescript_tests
status: passed
- task: lint_license_headers
status: passed
---
Coverage Report
The above coverage report was generated for the changes in this PR. |
|
Hi @kgryte, Please review my PR. thanks |
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
- task: lint_filenames
status: passed
- task: lint_editorconfig
status: passed
- task: lint_markdown
status: na
- task: lint_package_json
status: na
- task: lint_repl_help
status: na
- task: lint_javascript_src
status: na
- task: lint_javascript_cli
status: na
- task: lint_javascript_examples
status: passed
- task: lint_javascript_tests
status: na
- task: lint_javascript_benchmarks
status: na
- task: lint_python
status: na
- task: lint_r
status: na
- task: lint_c_src
status: na
- task: lint_c_examples
status: na
- task: lint_c_benchmarks
status: na
- task: lint_c_tests_fixtures
status: na
- task: lint_shell
status: na
- task: lint_typescript_declarations
status: na
- task: lint_typescript_tests
status: passed
- task: lint_license_headers
status: passed
---
Signed-off-by: Athan <[email protected]>
stats/base/nanminstats/base/nanmin
Signed-off-by: Athan <[email protected]>
Signed-off-by: Athan <[email protected]>
| x[ i ] = round( (randu()*100.0) - 50.0 ); | ||
| } | ||
| } | ||
| var x = discreteUniform( 10, -50, 50, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How you refactored this example does not preserve the original logic. See, e.g., https://github.com/stdlib-js/stdlib/tree/develop/lib/node_modules/%40stdlib/stats/strided/dnanmax#examples
| x[ i ] = round( (randu()*100.0) - 50.0 ); | ||
| } | ||
| } | ||
| var x = discreteUniform( 10, -50, 50, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as README.
| /** | ||
| * Computes the minimum value of a strided array, ignoring `NaN` values. | ||
| * | ||
| * @param {PositiveInteger} N - number of indexed elements |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * @param {PositiveInteger} N - number of indexed elements | |
| * @private | |
| * @param {PositiveInteger} N - number of indexed elements |
This function is not part of the package's public API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind renaming this file to main.js and updating index.js accordingly?
| * var x = [ 1.0, -2.0, NaN, 2.0 ]; | ||
| * var N = x.length; | ||
| * | ||
| * var v = nanmin( N, x, 1 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go ahead and remove L39 and replace N here with x.length. The use of N in this example doesn't really add anything.
| * var x = [ 2.0, 1.0, 2.0, -2.0, -2.0, 2.0, 3.0, 4.0, NaN, NaN ]; | ||
| * var N = floor( x.length / 2 ); | ||
| * | ||
| * var v = nanmin( N, x, 2, 1 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to replace usage of floor in this example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That may also apply to index.js.
| var toAccessorArray = require( '@stdlib/array/base/to-accessor-array' ); | ||
| var isnan = require( '@stdlib/math/base/assert/is-nan' ); | ||
| var isNegativeZero = require( '@stdlib/math/base/assert/is-negative-zero' ); | ||
| var Float64Array = require( '@stdlib/array/float64' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
L28 will need to be updated after renaming nanmin.js. May apply elsewhere, as well.
Signed-off-by: Athan <[email protected]>
kgryte
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left an initial round of comments.
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
- task: lint_filenames
status: passed
- task: lint_editorconfig
status: passed
- task: lint_markdown
status: passed
- task: lint_package_json
status: na
- task: lint_repl_help
status: na
- task: lint_javascript_src
status: passed
- task: lint_javascript_cli
status: na
- task: lint_javascript_examples
status: passed
- task: lint_javascript_tests
status: passed
- task: lint_javascript_benchmarks
status: passed
- task: lint_python
status: na
- task: lint_r
status: na
- task: lint_c_src
status: na
- task: lint_c_examples
status: na
- task: lint_c_benchmarks
status: na
- task: lint_c_tests_fixtures
status: na
- task: lint_shell
status: na
- task: lint_typescript_declarations
status: na
- task: lint_typescript_tests
status: na
- task: lint_license_headers
status: passed
---
|
Hi @kgryte, I have addressed requested changes. |
Signed-off-by: Athan <[email protected]>
|
Thank you for working on this pull request. However, we cannot accept your contribution as the issue this pull request seeks to resolve has already been addressed in a different pull request or commit. Thank you again for your interest in stdlib, and we look forward to reviewing your future contributions. |
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes. report:
Resolves #5659 .
Description
This pull request:
lib/accessors.jsfile.lib/nanmin.jsandlib/ndarray.js.Related Issues
This pull request:
stats/base/nanmin#5659Questions
No.
Other
No.
Checklist
@stdlib-js/reviewers